Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove redundant Prometheus variable labels #5008

Merged
merged 3 commits into from
Feb 6, 2024
Merged

Remove redundant Prometheus variable labels #5008

merged 3 commits into from
Feb 6, 2024

Conversation

haywoodsh
Copy link
Contributor

@haywoodsh haywoodsh commented Jan 30, 2024 •
edited
Loading

Proposed changes

Remove unused Prometheus variable labels to eliminate warning messages from Prometheus exporter. closes #4952

The worker metrics now looks like the following without any warning messages

Tests

worker

In the main branch, there were warning messages when prometheus metrics are exported. The following logs are no longer found.

level=warn ts=2024-01-30T17:18:40.810Z caller=nginx_plus.go:1258 wrongnumberoflabelsforworker%v.Forlabels%v,gotvalues:%v.Emptylabelswillbeusedinstead=0
level=warn ts=2024-01-30T17:18:40.811Z caller=nginx_plus.go:1258 wrongnumberoflabelsforworker%v.Forlabels%v,gotvalues:%v.Emptylabelswillbeusedinstead=1
level=warn ts=2024-01-30T17:18:40.812Z caller=nginx_plus.go:1258 wrongnumberoflabelsforworker%v.Forlabels%v,gotvalues:%v.Emptylabelswillbeusedinstead=2
level=warn ts=2024-01-30T17:18:40.812Z caller=nginx_plus.go:1258 wrongnumberoflabelsforworker%v.Forlabels%v,gotvalues:%v.Emptylabelswillbeusedinstead=3
level=warn ts=2024-01-30T17:18:40.812Z caller=nginx_plus.go:1258 wrongnumberoflabelsforworker%v.Forlabels%v,gotvalues:%v.Emptylabelswillbeusedinstead=4
level=warn ts=2024-01-30T17:18:40.812Z caller=nginx_plus.go:1258 wrongnumberoflabelsforworker%v.Forlabels%v,gotvalues:%v.Emptylabelswillbeusedinstead=5

In the main branch, extra labels resource_name ,resource_namespace, resource_type are found in the prometheus metrics with no values.

# HELP nginx_ingress_nginxplus_worker_connection_accepted The total number of accepted client connections
# TYPE nginx_ingress_nginxplus_worker_connection_accepted counter
nginx_ingress_nginxplus_worker_connection_accepted{class="nginx",id="0",pid="60",resource_name="",resource_namespace="",resource_type=""} 3
nginx_ingress_nginxplus_worker_connection_accepted{class="nginx",id="1",pid="50",resource_name="",resource_namespace="",resource_type=""} 0
nginx_ingress_nginxplus_worker_connection_accepted{class="nginx",id="2",pid="52",resource_name="",resource_namespace="",resource_type=""} 0
nginx_ingress_nginxplus_worker_connection_accepted{class="nginx",id="3",pid="54",resource_name="",resource_namespace="",resource_type=""} 0
nginx_ingress_nginxplus_worker_connection_accepted{class="nginx",id="4",pid="55",resource_name="",resource_namespace="",resource_type=""} 0
nginx_ingress_nginxplus_worker_connection_accepted{class="nginx",id="5",pid="57",resource_name="",resource_namespace="",resource_type=""} 0

In this branch, worker metrics exported in this branch no longer contain the redundant values:

# HELP nginx_ingress_nginxplus_worker_connection_accepted The total number of accepted client connections
# TYPE nginx_ingress_nginxplus_worker_connection_accepted counter
nginx_ingress_nginxplus_worker_connection_accepted{class="nginx",id="0",pid="44"} 3
nginx_ingress_nginxplus_worker_connection_accepted{class="nginx",id="1",pid="46"} 0
nginx_ingress_nginxplus_worker_connection_accepted{class="nginx",id="2",pid="48"} 0
nginx_ingress_nginxplus_worker_connection_accepted{class="nginx",id="3",pid="49"} 0
nginx_ingress_nginxplus_worker_connection_accepted{class="nginx",id="4",pid="52"} 0
nginx_ingress_nginxplus_worker_connection_accepted{class="nginx",id="5",pid="53"} 0
http cache

No changes in logs and metrics as http cache variable labels are not processed in the prometheus exporter.

# HELP nginx_ingress_nginxplus_cache_bypass_bytes Total number of bytes returned from cache bypasses
# TYPE nginx_ingress_nginxplus_cache_bypass_bytes counter
nginx_ingress_nginxplus_cache_bypass_bytes{class="nginx",zone="jwk"} 0
nginx_ingress_nginxplus_cache_bypass_bytes{class="nginx",zone="jwks_uri_webapp"} 0

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto main
  • I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

@github-actions github-actions bot added the bug An issue reporting a potential bug label Jan 30, 2024
@codecov Codecov
Copy link

codecov bot commented Jan 30, 2024 •
edited
Loading

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (8a2c9a9) 52.32% compared to head (d576d0b) 52.31%.
Report is 5 commits behind head on main.

❗ Current head d576d0b differs from pull request most recent head e7ebfa5. Consider uploading reports for the commit e7ebfa5 to get more accurate results

Files Patch % Lines
cmd/nginx-ingress/main.go 0.00% 1 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #5008      +/-   ##
==========================================
- Coverage   52.32%   52.31%   -0.01%     
==========================================
  Files          61       60       -1     
  Lines       17502    17499       -3     
==========================================
- Hits         9158     9155       -3     
- Misses       8015     8016       +1     
+ Partials      329      328       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@haywoodsh haywoodsh marked this pull request as ready for review January 30, 2024 14:41
@haywoodsh haywoodsh requested a review from a team as a code owner January 30, 2024 14:41
@haywoodsh haywoodsh force-pushed the fix/remove-prometheus-exporter-warning-logs branch from d576d0b to bef63eb Compare January 30, 2024 17:31
@haywoodsh
Remove redundant variable labels
620da3a 
Signed-off-by: Haywood Shannon <[email protected]>

Signed-off-by: Haywood Shannon <[email protected]>
@haywoodsh haywoodsh force-pushed the fix/remove-prometheus-exporter-warning-logs branch from bef63eb to 620da3a Compare January 30, 2024 18:52
@haywoodsh haywoodsh requested a review from a team February 1, 2024 17:43
@haywoodsh haywoodsh enabled auto-merge (squash) February 6, 2024 12:13
@haywoodsh haywoodsh merged commit fb60fb6 into main Feb 6, 2024
49 of 52 checks passed
@haywoodsh haywoodsh deleted the fix/remove-prometheus-exporter-warning-logs branch February 6, 2024 12:24
haywoodsh added a commit that referenced this pull request Feb 13, 2024
@haywoodsh
Remove redundant Prometheus variable labels (#5008)
c0dc07c 
Remove redundant variable labels

Signed-off-by: Haywood Shannon <[email protected]>
(cherry picked from commit fb60fb6)
pdabelf5 pushed a commit that referenced this pull request Feb 13, 2024
@haywoodsh
Remove redundant Prometheus variable labels (#5008) (#5085)
0f45ece 
Remove redundant variable labels

Signed-off-by: Haywood Shannon <[email protected]>
(cherry picked from commit fb60fb6)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jjngx jjngx jjngx approved these changes

@vepatel vepatel vepatel approved these changes

@oseoin oseoin oseoin approved these changes

Assignees
No one assigned
Labels
bug An issue reporting a potential bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Eliminate Warning Logs When Prometheus Metrics are Enabled
4 participants
@haywoodsh @oseoin @vepatel @jjngx